add test for non-admin user #415

gouyang · 2019-07-03T07:26:02Z

Implementation of Polarion test case:
https://polarion.engineering.redhat.com/polarion/#/project/CNV/workitem?id=CNV-1718
https://polarion.engineering.redhat.com/polarion/#/project/CNV/workitem?id=CNV-1720

Comments for review:

It's using UI to create the test project, it's more convenient than do it via cli.

Signed-off-by: Guohua Ouyang gouyang@redhat.com

rhrazdil

Could you please add links to polarion test plans for the implemented test cases?

frontend/integration-tests/views/kubevirt/login.view.ts

rhrazdil · 2019-07-13T13:54:24Z

frontend/integration-tests/tests/kubevirt/kubevirt.login.scenario.ts

-    if (process.env.BRIDGE_BASE_ADDRESS !== undefined) {
-      await logIn();
-      execSync(`oc login -u ${process.env.BRIDGE_AUTH_USERNAME} -p ${process.env.BRIDGE_AUTH_PASSWORD} --config=${process.env.KUBECONFIG}`);
+    if (await $('a.idp').isPresent()) {


I would prefer moving this logic to login method in utils, the login method should be improved to handle

logging in kubeadmin (in both cases - with and without htpasswd identity provider enabled),

logging in a different non-admin user (atm it's enough to only consider htpasswd)

it must also take into account that when tests are executed with bridge UI, login screen is not displayed (so in that case it shouldn't be waiting for login form to appear)

Put this logic into views/login.view.ts and simply the login function now.

frontend/integration-tests/tests/kubevirt/kubevirt.login.scenario.ts

frontend/integration-tests/tests/kubevirt/non-admin.scenario.ts

rhrazdil · 2019-07-13T14:18:14Z

frontend/integration-tests/tests/kubevirt/non-admin.scenario.ts

+  });
+
+  afterAll(async() => {
+    await logout();


When running with bridge, there is no logout button, it should be enough to just do oc login with different user and reload page.

oc login with different user and reload page does not work, it's not logging in a different user on UI.

frontend/integration-tests/tests/kubevirt/non-admin.scenario.ts

atiratree · 2019-07-15T11:43:48Z

frontend/integration-tests/tests/kubevirt/non-admin.scenario.ts

+  });
+
+  it('Nonadmin user with RoleBinding edit can create/edit vm objetcs in other NS', async() => {
+    execSync(`oc adm policy add-role-to-user edit ${BRIDGE_HTPASSWD_USERNAME} -n ${testName}`);


this could be simplified by a function; e.g. called like this:

withRole(role, name, namespace, () => { await browser.get(`${appHost}/k8s/ns/${testName}/virtualmachines`); await crudView.isLoaded(); ... })

and similarly

withClusterRole(...

atiratree · 2019-07-15T11:55:52Z

frontend/integration-tests/tests/kubevirt/non-admin.scenario.ts

+  it('Nonadmin user with RoleBinding edit can create/edit vm objetcs in other NS', async() => {
+    execSync(`oc adm policy add-role-to-user edit ${BRIDGE_HTPASSWD_USERNAME} -n ${testName}`);
+
+    await browser.get(`${appHost}/k8s/ns/${testName}/virtualmachines`);


the inside of this method can be a parametrized function of 'Nonadmin user can use vm dialog in its own namespace'

atiratree · 2019-07-15T11:56:14Z

frontend/integration-tests/tests/kubevirt/non-admin.scenario.ts

+  it('Nonadmin user with ClusterRoleBinding view can view vm objetcs in the cluster', async() => {
+    execSync(`oc adm policy add-cluster-role-to-user view ${BRIDGE_HTPASSWD_USERNAME}`);
+
+    await browser.get(`${appHost}/k8s/ns/${testName}/virtualmachines`);


duplicate implementation

atiratree · 2019-07-15T11:56:20Z

frontend/integration-tests/tests/kubevirt/non-admin.scenario.ts

+  it('Nonadmin user with ClusterRoleBinding edit can create/edit vm objetcs in the cluster', async() => {
+    execSync(`oc adm policy add-cluster-role-to-user edit ${BRIDGE_HTPASSWD_USERNAME}`);
+
+    await browser.get(`${appHost}/k8s/ns/${testName}/virtualmachines`);


atiratree · 2019-07-15T11:58:15Z

Also, there is other functionality which should be tested for permissions / missing resources.
This functionality could be a nice followup

gouyang · 2019-07-22T12:58:15Z

@suomiy @rhrazdil I made some changes, could you please review again?

atiratree · 2019-07-23T17:03:42Z

frontend/integration-tests/tests/kubevirt/non-admin.scenario.ts

+  const nonAdminNS = `${testName}-nonadmin`;
+  const userRole = ['view', 'edit'];
+
+  const verifyPermissions = async function( perm: string, ns: string) {


What is the benefit of this to be an inner function?

I see some same usage about the inner function in modal-annotations.scenario.ts, so just try to use the similar thing here. Is it so bad?

It is not that bad (:, just it would be easier to potentially reuse this function in the future.

atiratree · 2019-07-23T17:04:22Z

frontend/integration-tests/tests/kubevirt/non-admin.scenario.ts

+      await click(crudView.createItemButton);
+      await click(createWithWizardLink);
+      await crudView.isLoaded();
+      await click(closeWizard);


shouldn't we also check the yaml in VM Templates?

The VM and templates are very similar. Can you please make it generic?

it's checking whether user has permission to use wizard dialog and yaml dialog, VM has 'create from yaml' dialog but VM template doesn't have it.

VM Template newly received this functionality in https://github.com/openshift/console

atiratree · 2019-07-23T17:05:58Z

frontend/integration-tests/tests/kubevirt/non-admin.scenario.ts

+      await click(closeWizard);
+    }
+
+    await browser.get(`${appHost}/overview/ns/${ns}/`);


this block would look better extracted into a function

atiratree · 2019-07-23T17:09:07Z

frontend/integration-tests/tests/kubevirt/non-admin.scenario.ts

+    }
+  };
+
+  const verifyPermissionWithRoleBinding = async function( perm: string, ns: string, roleBinding: string ) {


can you please move the perm arg to the end - it is easier to follow a pattern like this:
f(what, options)

again, why inside?

Will move the 'perm' arg to the end.

frontend/integration-tests/tests/kubevirt/non-admin.scenario.ts

frontend/integration-tests/tests/kubevirt/utils/consts.ts

mareklibra · 2019-07-24T12:14:04Z

@gouyang , have you considered implementing new tests in openshift-console, please?

There will be no other build of web-ui (except bug-fixes for 2.0.0). And as we have migrated almost all features from here to upstream already, we are moving integration tests is in progress as well these days.

So targeting openshift-console directly sounds more effective to me atm.

rhrazdil · 2019-07-24T09:08:09Z

frontend/integration-tests/tests/kubevirt/kubevirt.login.scenario.ts

-      await logIn();
-      execSync(`oc login -u ${process.env.BRIDGE_AUTH_USERNAME} -p ${process.env.BRIDGE_AUTH_PASSWORD} --config=${process.env.KUBECONFIG}`);
-    }
+    await browser.sleep(3000); // Wait long enough for the login redirect to complete


Could we rather wait for some element to appear?

I copied this from login.scenario.ts. It seems no necessary to sleep here as there are await in loginView.login.

rhrazdil · 2019-07-24T09:21:49Z

frontend/integration-tests/tests/kubevirt/non-admin.scenario.ts

+      await browser.sleep(6000);
+      expect(crudView.errorMessage.getText()).toContain('cannot list resource');
+
+      // TODO: check it can't use vm dialog once BZ1728523 is fixed.


Wouldn't it be better if different permissions were tested in separate specs?
This verifyPermissions method seems too big and has no versatility. If there is a bug in some of the permissions, there is no way to specifically skip it.
This way we have passing something that should fail. It's better to skip a failing test with xit, as it's visible in test report.

Separate specs means different describe, right?
I can change it like:

describe('Test nonadmin user UI behavior', () => { describe('Test nonadmin user with roleBinding view UI behavior', () => { describe('Test nonadmin user with roleBinding edit UI behavior', () => { describe('Test nonadmin user with clusterRoleBinding view UI behavior', () => { describe('Test nonadmin user with clusterRoleBinding edit UI behavior', () => {

And it does not need to change verifyPermissions.

No, spec is it block

Okay, I will make changes to the PR in console.

rhrazdil · 2019-07-24T09:26:40Z

frontend/integration-tests/tests/kubevirt/non-admin.scenario.ts

+    await browser.get(`${appHost}/k8s/ns/${ns}/virtualmachines`);
+    if (perm === 'noAccess') {
+      // crudView.isLoaded doesn't work here.
+      await browser.sleep(6000);


It would be better to just wait for the 'cannot list resource text' text. Check waitForStringInElement method in utils.ts and how it's used

Change it another way in openshift#2265

rhrazdil · 2019-07-24T09:34:50Z

frontend/integration-tests/tests/kubevirt/non-admin.scenario.ts

+      // TODO: check it can't use vm dialog once BZ1728523 is fixed.
+    }
+    if (perm === 'edit') {
+      await crudView.isLoaded();


Use Wizard instance for this:

const wizard = new Wizard(); await wizard.openWizard(); await wizard.closeWizard();

I believe there are similar methods in the Yaml class

Updated in openshift#2265

rhrazdil · 2019-07-24T10:38:03Z

frontend/integration-tests/tests/kubevirt/non-admin.scenario.ts

+  it('Login with nonadmin', async() => {
+    await logout();
+    // wait for logout complete
+    await browser.sleep(10000);


Here we should rather wait for login screen with kubeadmin/httpass provider selection

It can use that once 1721423 isn't blocking it.

rhrazdil · 2019-07-24T10:40:11Z

frontend/integration-tests/tests/kubevirt/non-admin.scenario.ts

+  });
+
+  it('Nonadmin can use vm dialog in its own namespace but not other namespace', async() => {
+    await verifyPermissions('edit', nonAdminNS);


This I think should be broken down into multiple specs

gouyang · 2019-07-29T05:14:42Z

@gouyang , have you considered implementing new tests in openshift-console, please?

There will be no other build of web-ui (except bug-fixes for 2.0.0). And as we have migrated almost all features from here to upstream already, we are moving integration tests is in progress as well these days.

So targeting openshift-console directly sounds more effective to me atm.

I'm okay to move this to openshift-console, will move it over after address the review comments.

Signed-off-by: Guohua Ouyang <gouyang@redhat.com>

rhrazdil · 2019-08-07T05:43:53Z

frontend/integration-tests/tests/kubevirt/utils/utils.ts


 export function createResource(resource) {
-  execSync(`echo '${JSON.stringify(resource)}' | kubectl create -f -`);
+  execSync(`echo '${JSON.stringify(resource)}' | kubectl apply -f -`);


Is this a good idea to mix declarative and imperative approach in creating resources? Wouldn't it be safer to have a new method, i.e applyResource?

Well, the purpose of this change is avoiding to create a new method. Of course it can has another method or maybe just use execSync.

And why do we need createResource for one line code?

rhrazdil · 2019-08-07T05:44:25Z

frontend/integration-tests/views/login.view.ts


 export const login = async(providerName: string, username: string, password: string) => {
-  if (providerName) {
+  if (await $('a.idp').isPresent()) {


Note that this change probably should not be necessary in openshift/console

Without this change, it can't run test with a cluster without idp.
It's not necessary if the test run with idp always, I know it's not perfect.

I have a PR in console about this: openshift#2016

gouyang · 2019-08-07T06:23:30Z

Moved this to openshift#2265, will close this.

gouyang changed the title ~~[WIP]add test for non-admin user~~ add test for non-admin user Jul 10, 2019

kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 10, 2019

rhrazdil suggested changes Jul 14, 2019

View reviewed changes

atiratree suggested changes Jul 15, 2019

View reviewed changes

atiratree reviewed Jul 23, 2019

View reviewed changes

rhrazdil suggested changes Jul 26, 2019

View reviewed changes

add test for non-admin user

93013bf

Signed-off-by: Guohua Ouyang <gouyang@redhat.com>

rhrazdil reviewed Aug 7, 2019

View reviewed changes

gouyang closed this Aug 7, 2019

add test for non-admin user #415

add test for non-admin user #415

Uh oh!

Conversation

gouyang commented Jul 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rhrazdil left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atiratree commented Jul 15, 2019

Uh oh!

gouyang commented Jul 22, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mareklibra commented Jul 24, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gouyang Aug 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gouyang commented Jul 3, 2019 •

edited

Loading

gouyang Aug 7, 2019 •

edited

Loading